Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Kirby v4 #52

Merged
merged 10 commits into from
Nov 3, 2023
Merged

Support Kirby v4 #52

merged 10 commits into from
Nov 3, 2023

Conversation

johannschopplich
Copy link
Contributor

@johannschopplich johannschopplich commented Oct 2, 2023

This PR 1️⃣ renames the Content and Field class aliases:

  • Kirby\Cms\Field class to Kirby\Content\Field
  • Kirby\Cms\Content class to Kirby\Content\Content

While 2️⃣ adding an interceptor alias to keep support for Kirby 3.

@lukasbestle lukasbestle linked an issue Oct 3, 2023 that may be closed by this pull request
@lukasbestle
Copy link
Member

Thanks for your PR:

I think we could create an alias class for the old Cms class names. This way we will be able to keep compatibility with v3.

So something like:

<?php

namespace Kirby\Kql\Interceptors\Cms;

use Kirby\Kql\Interceptors\Content\Content as NewContent;

class Content extends NewContent
{
}

@johannschopplich
Copy link
Contributor Author

Thanks for the hint! To add backwards compatibility to my Kirby 3 projects, I currently rely on a similar class alias for my libraries, which basically only require one change for Kirby 4 compatibility:

// Add backwards compatibility for Kirby 3
if (!class_exists('Kirby\Content\Field')) {
    class_alias('Kirby\Cms\Field', 'Kirby\Content\Field');
}

I'm not sure my PHP knowledge will be sufficient to replicate that using your approach for my PR, but I will give it a shot.

@johannschopplich
Copy link
Contributor Author

Thanks for the ideas @lukasbestle, which I have implemented accordingly. But I'm running into a problem with Psalm that I can't seem to solve. Perhaps the solution is obvious to you.

@lukasbestle
Copy link
Member

Hm, good point. Maybe a class alias could actually work as well:

use Kirby\Kql\Interceptors\Content\Content;
use Kirby\Kql\Interceptors\Content\Field;

class_alias(Content::class, 'Kirby\Kql\Interceptors\Cms\Content');
class_alias(Field::class, 'Kirby\Kql\Interceptors\Cms\Field');

@lukasbestle
Copy link
Member

Could you please revert composer.json and composer.lock to the currently released state? I think it would be good to keep the version requirements unchanged unless we need to.

@johannschopplich
Copy link
Contributor Author

For sure! Done.

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your work. 💛

Code-wise it looks good to me. Waiting for a second review from Bastian or Nico to confirm that this is the correct solution. Also it still needs real-world testing.

@johannschopplich
Copy link
Contributor Author

johannschopplich commented Oct 3, 2023

Great! Thanks for the mentoring on this PR, @lukasbestle. Pretty insightful. 🙂

Regarding real-live testing, I'm using my fork (on which this PR is based on) for both K3 in production and a K4 test site. I can verify it works.

This is my setup (relevant lines extracted):

{
  "require": {
    "getkirby/cms": "^3.9",
    "getkirby/kql": "dev-v4-compat"
  },
  "repositories": [
    {
      "url": "https://github.com/johannschopplich/kql-fork.git",
      "type": "git"
    }
  ]
}

Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy with this. Thanks for the PR!

@bastianallgeier bastianallgeier merged commit fc9fe76 into getkirby:main Nov 3, 2023
4 checks passed
@bastianallgeier bastianallgeier added this to the 2.1.0 milestone Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kirby 4 compatibility
3 participants